perf: single dispute-chat REQ and constant-time session resolution - #714
Conversation
The dispute chat notifier opened its own kind-14 REQ per dispute - a duplicate of the one SubscriptionManager already maintains for SubscriptionType.disputeChat, whose stream nobody consumed - and resolved its session by scanning every session and instantiating an OrderNotifier (DB sync, storage watcher, book listener) per candidate, on every incoming event, send and read-status check. - The notifier now consumes the manager's shared disputeChat stream (per-dispute filtering stays in the existing K_sign pre-filter), so one REQ serves all disputes and the manager's persisted shared cursor bounds the replay. - _getSessionForDispute resolves through the persisted session.disputeId first (constant time, side-effect free); the order-state scan remains only as a fallback for sessions persisted before disputeId existed. - Subscription.cancel tolerates teardown ordering: disposing the container while REQs are open no longer throws from onCancel.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c7b09c9ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
Request changes
The session-resolution half is solid and I'd merge it as is. The single-REQ half
introduces a message-visibility regression that needs to be closed first.
What I verified
flutter analyze: clean.disputes/,subscriptions/andchat/suites: green except
dispute_chat_duplicate_envelope_test, which I confirmed also fails on the
base commitef3aad30— the known pre-existing failure fixed by #710.- The new test is well designed: the throwing
orderNotifierProvideroverride
pins the "no OrderNotifier instantiation" guarantee properly. session.disputeIdis genuinely persisted (dispute_idintoJson) and set on
both dispute-initiation paths (abstract_mostro_notifier.dart:564and:653),
so the fast path is correct and the fallback covers legacy sessions.SubscriptionManagerdoes rebuild the REQ when a dispute starts mid-session:
_emitState()always assigns a fresh list, the listener fires, and
_filterIdentitychanges because it includes theadminSharedKeypublics. No
risk of a dispute ending up with no subscription at all.
Blocking: the backfill on opening a dispute chat is lost
DisputeChatNotifier is created lazily — only when DisputesList renders,
which lives behind the "Disputes" tab (chat_rooms_list.dart:94). That did not
matter before, because on creation the notifier opened its own REQ with
since = persisted cursor and the relay replayed whatever had been missed.
Now it only listens on a StreamController.broadcast(), and a broadcast
controller drops events while it has no listener. So every kind-14 envelope
delivered on the shared REQ before the user enters the Disputes tab is discarded:
not displayed, not written to disk, and the cursor does not advance. That includes
the backlog the relay replays when the REQ is issued at app start.
I reproduced it by taking the PR's own test and pushing the envelope before
creating the notifier: Expected: length of <1>, Actual: [].
Contrast with P2P chat, where this hole does not exist: app_init_provider.dart:60-62
eagerly creates a ChatRoomNotifier per session with a peer, so the shared chat
stream always has a listener. Disputes have no equivalent.
To be fair on severity: the message is not lost forever. A background/foreground
cycle recovers it — the background service subscribes with the persisted filters,
decrypts and stores it (background_notification_service.dart:314), and
_switchToForeground invalidates the family so it reloads from disk. But in the
meantime the user opens their dispute and does not see the admin's latest message,
which does not happen today.
Suggested fix, either one:
- Mirror the P2P pattern: create the notifier in
app_init_providerfor sessions
withdisputeId != null. Two lines, no new API — my preference. - Or have
_subscribe()ask the manager to re-issue thedisputeChatREQ once
when it attaches, so the relay replays the backlog with the listener connected.
Still a single REQ.
Plus a test pinning it: an envelope delivered on the shared stream before the
notifier exists must still end up visible.
Non-blocking nits
- The
try/catchinonCancel(subscription_manager.dart:471) swallows any
error, not just theStateErrorfrom a disposed container. Ifunsubscribe()
ever failed for another reason the relay CLOSE is silently skipped and the REQ
lingers — exactly the waste this PR removes. It is logged, so it is tolerable,
but narrowing thetryto theref.readwould be more honest. lifecycle_manager.dart:120-122is now half stale: the notifier no longer
"re-opens its relay subscription" because it never opens one.- Worth noting the two halves differ a lot in payoff: the duplicate REQ is one per
relay and only while a dispute chat is open, whereas taking
ref.read(orderNotifierProvider(...))off the per-event path is the substantial
win.
|
@coderabbitai review |
|
`SubscriptionManager.disputeChat` is a broadcast stream, and a broadcast stream drops events while nothing is listening. `DisputeChatNotifier` is built lazily — only when the Disputes tab renders — so every envelope the shared REQ delivered before that was discarded: not displayed, not persisted, and the cursor never advanced. That includes the backlog the relay replays when the REQ is first issued. Before this PR the notifier opened its own REQ on creation, so the relay always backfilled it. Adds `SubscriptionManager.refreshDisputeChatSubscription()`, asked for once by the notifier when it attaches: it clears the applied filter key and re-issues the REQ, so the relay replays from the persisted cursor with a listener connected. Still a single REQ — re-issued once, when a consumer shows up — and it also covers a dispute that starts mid-run, which eager notifier creation at startup would miss. Narrows the `onCancel` guard to the `ref.read`, per review: a failing `unsubscribe()` means the relay CLOSE was skipped and the REQ lingers, which is exactly the waste this PR removes, so it must surface rather than be swallowed. Doing so exposed a latent NPE — dart_nostr only assigns `subscriptionId` when it serializes the REQ onto a socket, so a request that never reached a relay has none — now handled as "nothing to CLOSE". Also refreshes the now-stale dispute comment in the foreground transition.
|
Gracias @Catrya — el bloqueante es real y lo reproduje antes de tocar nada. @chatgpt-codex-connector llegó al mismo P1 de forma independiente, así que lo trato como un único hallazgo. Arreglado en 65ab00f. Bloqueante: se perdía el backfill al abrir el chat de disputa — CONFIRMADO y arregladoVerificado punto por punto:
Fix elegido: la opción (b), catch-up al enganchar. Preferí ésta sobre la creación eager en Nuevo Test, calcado de tu reproducción: Nit del
|
Summary
Testing
|
2 participants
Summary
Item 4.5 (last of Phase 4) of the performance plan. Two dispute-chat wastes:
DisputeChatNotifieropened its own kind-14 REQ — a duplicate of the oneSubscriptionManageralready maintains forSubscriptionType.disputeChat, whose stream nobody consumed._getSessionForDisputescanned every session andref.read(orderNotifierProvider(id))per candidate — instantiating anOrderNotifier(DB sync + storage watcher + book listener) for any session that didn't have one — on every incoming event, send, and read-status check.Changes
disputeChatbroadcast (the existing K_sign pre-filter keeps per-dispute isolation); one REQ serves all disputes, bounded by the manager's shared persisted cursor._getSessionForDisputeresolves via the persistedsession.disputeIdfirst (constant time, side-effect free — pinned by a test whoseorderNotifierProvideroverride throws); the order-state scan stays only as a fallback for sessions persisted beforedisputeIdexisted.Subscription.canceltolerates teardown ordering (container disposal with open REQs no longer throws fromonCancel) — surfaced by the reload test once the real manager participates in its container.Test plan
dispute_chat_single_req_test.dart(RED onmain): an envelope pushed through the shared manager stream reaches the notifier, withorderNotifierProvideroverridden to throwdispute_chat_duplicate_envelope_test(fixed by test: fix the duplicate-envelope race pin for cross-isolate unwrapping #710, not in this base)flutter analyze— no new issues🤖 Generated with Claude Code
https://claude.ai/code/session_018fTxqxhpdL5siTgKZqwtur